Determine default targets from CMAKE_RN_TARGETS and FERRIC_TARGETS environment variable#191
Conversation
kraenhansen
added
enhancement
🦋 Changeset detectedLatest commit: 9852f7e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
kraenhansen
added
Apple 🍎
kraenhansen
force-pushed
the
kh/default-targets-from-env
branch
from
70624d4 to
ee234b5
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces environment variable-based default target configuration for both cmake-rn and ferric CLIs. The purpose is to allow users to specify default build targets via environment variables rather than requiring explicit command-line arguments each time.
- Adds
CMAKE_RN_TARGETSenvironment variable support for cmake-rn CLI default targets - Adds
FERRIC_TARGETSenvironment variable support for ferric CLI default targets - Enhances CLI output to show platform and target information during project configuration
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cmake-rn/src/cli.ts | Implements CMAKE_RN_TARGETS parsing, validation, and adds target summary display |
| packages/ferric/src/build.ts | Implements FERRIC_TARGETS parsing with validation using assertFixable |
| .github/workflows/check.yml | Updates CI configuration to use the new environment variables |
| .changeset/loud-paths-eat.md | Documents ferric-cli changes |
| .changeset/loud-dodos-kneel.md | Documents cmake-rn changes |
|
|
||
| const defaultTargets = CMAKE_RN_TARGETS ? CMAKE_RN_TARGETS.split(",") : []; | ||
|
|
||
| for (const target of defaultTargets) { |
There was a problem hiding this comment.
The target validation logic is duplicated between cmake-rn and ferric packages. Consider extracting this validation into a shared utility function to maintain consistency and reduce code duplication.
| const { FERRIC_TARGETS } = process.env; | ||
|
|
||
| function getDefaultTargets() { |
There was a problem hiding this comment.
[nitpick] The environment variable extraction is placed at module level but only used within getDefaultTargets(). Consider moving this extraction inside the function to improve encapsulation and make the dependency more explicit.
| const { FERRIC_TARGETS } = process.env; | |
| function getDefaultTargets() { | |
| function getDefaultTargets() { | |
| const { FERRIC_TARGETS } = process.env; |
kraenhansen
changed the title
CMAKE_RN_TARGETS environment variableCMAKE_RN_TARGETS and FERRIC_TARGETS environment variable
shirakaba
left a comment
shirakaba
left a comment
There was a problem hiding this comment.
Some post-review questions.
| CMAKE_RN_TARGETS: arm64-apple-ios-sim | ||
| FERRIC_TARGETS: aarch64-apple-ios-sim |
There was a problem hiding this comment.
Is there any reason why Ferric handles the build of aarch64 while cmake-rn handles arm64?
There was a problem hiding this comment.
It's a naming difference between Xcode and the Rust compiler targets.
| const targetOption = new Option("--target <target...>", "Targets to build for") | ||
| .choices(allTargets) | ||
| .default( | ||
| defaultTargets, | ||
| "CMAKE_RN_TARGETS environment variable split by ','" | ||
| ); |
There was a problem hiding this comment.
When I last used cmake-rn@0.2.2, when invoking the cmake-rn and cmake-rn --android --ios commands, the CLI didn't prompt me for what targets to build. Is that still the case, or has something changed recently?
There was a problem hiding this comment.
There's another fallback default if no targets are provided through the CLI runtime options. The reason that is not handled here is because I wanted to print a warning in that case.
We're currently building for Android x86_64 while running an x86 emulator.
Merging this PR will:
--targetruntime option when running thecmake-rnandferricCLIs.cmake-rnCLI.